Federation: separate notion of zone-name & dns-suffix #35372

justinsb · 2016-10-22T15:45:30Z

We can put subdomains into hosted zones (for example,
foo.federation.example.com can be hosted in example.com)

By allowing sharing a common hosted zone, this means the user doesn't
have to do as much setup.

This change is

justinsb · 2016-10-22T15:46:02Z

This is splitting #34901 into logical parts. I haven't found a test that covers the generated DNS names yet, but I expect e2e will find it for me :-)

cc @quinton-hoole @madhusudancs

justinsb · 2016-10-23T15:29:09Z

Now with unit test :-)

justinsb · 2016-10-26T14:06:05Z

ping @quinton-hoole / @madhusudancs

ghost · 2016-10-26T14:49:06Z

federation/cmd/federation-controller-manager/app/options/options.go

@@ -39,6 +39,8 @@ type ControllerManagerConfiguration struct {
 	FederationName string `json:"federationName"`
 	// zone name, like example.com.
 	ZoneName string `json:"zoneName"`
+	// suffix to use for publishing federated services.
+	FederationDnsSuffix string `json:"federationDnsSuffix"`


nit: It's a suffix to the service name, but a prefix to the DNS zone. Somewhat confusing, perhaps. I'd suggest either adding an example to clarify, or renaming it something like DNSSubZone?

How about ServiceDnsSuffix? I think that is unambiguous?

(DNSSubZone ambiguity: I'm not sure if that would include ZoneName)

ghost · 2016-10-26T14:49:28Z

federation/cmd/federation-controller-manager/app/options/options.go

@@ -101,6 +103,7 @@ func (s *CMServer) AddFlags(fs *pflag.FlagSet) {
 	fs.Var(componentconfig.IPVar{Val: &s.Address}, "address", "The IP address to serve on (set to 0.0.0.0 for all interfaces)")
 	fs.StringVar(&s.FederationName, "federation-name", s.FederationName, "Federation name.")
 	fs.StringVar(&s.ZoneName, "zone-name", s.ZoneName, "Zone name, like example.com.")
+	fs.StringVar(&s.FederationDnsSuffix, "federation-dns-suffix", s.FederationDnsSuffix, "DNS Suffix to use for federation domain names.")


As per above comment

ghost · 2016-10-26T14:55:22Z

federation/pkg/federation-controller/service/dns.go

-func getDnsZone(dnsZoneName string, dnsZonesInterface dnsprovider.Zones) (dnsprovider.Zone, error) {
-	dnsZones, err := dnsZonesInterface.List()
+// getDnsZone returns the zone, as identified by zoneName
+func (s *ServiceController) getDnsZone() (dnsprovider.Zone, error) {


I don't really like making this is member function of ServiceController. It has nothing to do with service controllers. If anything it should be a member of DNSZone, or a util function. Can we revert the function signature back to what it was?

Sure... it's because in the next PR we'll add ID as well, but we can look at that when we come to it. I agree with you; it's not clear that even when we add ID is should be on ServiceController

ghost

LGTM barring minor nits. Feel free to apply the label once done.

justinsb · 2016-10-30T21:00:31Z

Applied the changes & fixed, but I don't want to apply lgtm until we figure out the right name for the dns suffix flag, because it's really hard to change later (and so easy to change now!)

Right now we have service-dns-suffix, do you like that @quinton-hoole ?

k8s-ci-robot · 2016-10-30T21:30:42Z

Jenkins verification failed for commit cb3269e9b8d26a80af950059e154daed109237c7. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

We can put subdomains into hosted zones (for example, foo.federation.example.com can be hosted in example.com) By allowing sharing a common hosted zone, this means the user doesn't have to do as much setup.

ghost · 2016-11-01T18:01:43Z

Yeah, that's fine @justinsb . Thanks for your patience on this one. LGTM.
Please feel free to prod me if I'm holding you up on any other reviews. Does #34901 still need review?

justinsb · 2016-11-02T13:35:34Z

Thanks @quinton-hoole . #34901 is the "big picture", but once this merges I'll split out the next piece which is to enable zone selection by id or name. I've assigned it to myself for now.

The third piece is to batch updates, which I think is useful, but just "icing on the cake". I guess we'll see where we are once after PR 2/3. I'll also see if we're hitting the delays - if so it might be more important.

justinsb · 2016-11-04T15:05:51Z

Manually removing do-not-merge. I think merge bot added it because of the lack of the label, but then deleted its comment but not the label :-(

k8s-github-robot · 2016-11-05T23:52:54Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2016-11-06T00:29:09Z

Automatic merge from submit-queue

googlebot added the cla: yes label Oct 22, 2016

k8s-github-robot assigned ghost Oct 22, 2016

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Oct 22, 2016

justinsb force-pushed the federation_dns_hosted_zone_1 branch 2 times, most recently from 19cb087 to 2f7ea65 Compare October 23, 2016 15:28

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 23, 2016

ghost reviewed Oct 26, 2016

View reviewed changes

ghost reviewed Oct 27, 2016

View reviewed changes

justinsb force-pushed the federation_dns_hosted_zone_1 branch from 2f7ea65 to 7828249 Compare October 30, 2016 20:59

justinsb force-pushed the federation_dns_hosted_zone_1 branch from 7828249 to cb3269e Compare October 30, 2016 21:01

Federation: separate notion of zone-name & dns-suffix

a3ba760

We can put subdomains into hosted zones (for example, foo.federation.example.com can be hosted in example.com) By allowing sharing a common hosted zone, this means the user doesn't have to do as much setup.

justinsb force-pushed the federation_dns_hosted_zone_1 branch from cb3269e to a3ba760 Compare October 31, 2016 00:50

ghost added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 1, 2016

k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 1, 2016

justinsb added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Nov 2, 2016

justinsb removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 4, 2016

justinsb added this to the v1.5 milestone Nov 5, 2016

justinsb added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Nov 5, 2016

k8s-github-robot merged commit 42fe4ab into kubernetes:master Nov 6, 2016

shashidharatd mentioned this pull request Nov 15, 2016

[Federation] Support configuring service-dns-suffix in kubefed #36647

Closed

chentao1596 mentioned this pull request Dec 5, 2016

WIP:kubelet: support multi-headers when getting pod from HTTP source #38089

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Federation: separate notion of zone-name & dns-suffix #35372

Federation: separate notion of zone-name & dns-suffix #35372

justinsb commented Oct 22, 2016 •

edited by k8s-oncall

justinsb commented Oct 22, 2016

justinsb commented Oct 23, 2016

justinsb commented Oct 26, 2016

ghost Oct 26, 2016

justinsb Oct 30, 2016

ghost Oct 26, 2016

ghost Oct 26, 2016 •

edited by ghost

justinsb Oct 30, 2016

ghost left a comment

justinsb commented Oct 30, 2016

k8s-ci-robot commented Oct 30, 2016

ghost commented Nov 1, 2016

justinsb commented Nov 2, 2016

justinsb commented Nov 4, 2016

k8s-github-robot commented Nov 5, 2016

k8s-github-robot commented Nov 6, 2016

Federation: separate notion of zone-name & dns-suffix #35372

Federation: separate notion of zone-name & dns-suffix #35372

Conversation

justinsb commented Oct 22, 2016 • edited by k8s-oncall

justinsb commented Oct 22, 2016

justinsb commented Oct 23, 2016

justinsb commented Oct 26, 2016

ghost Oct 26, 2016

Choose a reason for hiding this comment

justinsb Oct 30, 2016

Choose a reason for hiding this comment

ghost Oct 26, 2016

Choose a reason for hiding this comment

ghost Oct 26, 2016 • edited by ghost

Choose a reason for hiding this comment

justinsb Oct 30, 2016

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

justinsb commented Oct 30, 2016

k8s-ci-robot commented Oct 30, 2016

ghost commented Nov 1, 2016

justinsb commented Nov 2, 2016

justinsb commented Nov 4, 2016

k8s-github-robot commented Nov 5, 2016

k8s-github-robot commented Nov 6, 2016

justinsb commented Oct 22, 2016 •

edited by k8s-oncall

ghost Oct 26, 2016 •

edited by ghost